Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test: fix flaky test-http-writable-true-after-close #17952

Closed
wants to merge 1 commit into from
Closed

test: fix flaky test-http-writable-true-after-close #17952

wants to merge 1 commit into from

Conversation

apapirovski
Copy link
Member

Fixes: #16321

This will need full stress test CI, coming shortly. (But it did fix it on my macOS.)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jan 2, 2018
@apapirovski
Copy link
Member Author

apapirovski commented Jan 2, 2018 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/12373/
Stress Test CI:
https://ci.nodejs.org/job/node-stress-single-test/1593/ (green)
https://ci.nodejs.org/job/node-stress-single-test/1594/ (green)

@apapirovski apapirovski added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 3, 2018
mcollina
mcollina requested changes Jan 3, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm -1 for this change. This removes the scope of the test itself, we are better off removing it altogether.

I'm kind of concerned that this might hide some underlining bugs or memory leak. res should close/finish/error, why is this not happening?

@lpinca
Copy link
Member

lpinca commented Jan 3, 2018

res should close/finish/error, why is this not happening?

Test description is misleading then as it says

// res.writable should not be set to false after it has finished sending

and this change actually tests that.

@mcollina
Copy link
Member

mcollina commented Jan 3, 2018

// res.writable should not be set to false after it has finished sending

Means when the outer request res has transmitted data. This PR tests that when the inner request has ended, res is still writable.

@lpinca
Copy link
Member

lpinca commented Jan 3, 2018 •
edited
Loading

I think the assumption (@apapirovski please correct me if I'm wrong) is that on next tick after inner res has finished writing, outer res has also finished writing, but yes maybe this is a wrong assumption.

@apapirovski
Copy link
Member Author

I think the assumption (@apapirovski please correct me if I'm wrong) is that on next tick after inner res has finished writing, outer res has also finished writing, but yes maybe this is a wrong assumption.

That's correct and the test does fail on older Node versions.

@apapirovski
Copy link
Member Author

Also IMO if there's a bug hiding here then it's unrelated to the description of this test and should honestly have its own test. The fact that finished and close have to both be used, is indicative of this test being bit too dependent on timing.

@mcollina
Copy link
Member

mcollina commented Jan 3, 2018

It’s not a timing issue, there is something more subtle going on.

mcollina
mcollina approved these changes Jan 3, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@apapirovski apapirovski force-pushed the fix-test-writable-true-after-close branch from d685368 to 673544e Compare January 3, 2018 16:57
@apapirovski
Copy link
Member Author

@mcollina @lpinca PTAL, I've traced down the issue and adjusted the test. It appears that there are situations where the socket associated with res is closed before the callback for the get is called and as a result the event handlers are bound too late.

@apapirovski
Copy link
Member Author

Stress test CI: https://ci.nodejs.org/job/node-stress-single-test/1610/
CI: https://ci.nodejs.org/job/node-test-pull-request/12397/

mcollina
mcollina approved these changes Jan 3, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@BridgeAR
Copy link
Member

BridgeAR commented Jan 5, 2018

Landed in 9cef060

@BridgeAR BridgeAR closed this Jan 5, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 5, 2018
@apapirovski @BridgeAR
test: fix flaky http-writable-true-after-close
9cef060 
PR-URL: nodejs#17952
Fixes: nodejs#16321
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@apapirovski apapirovski deleted the fix-test-writable-true-after-close branch January 5, 2018 02:24
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
@apapirovski @MylesBorins
test: fix flaky http-writable-true-after-close
2ef944f 
PR-URL: #17952
Fixes: #16321
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@apapirovski @MylesBorins
test: fix flaky http-writable-true-after-close
dec917f 
PR-URL: #17952
Fixes: #16321
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@apapirovski @MylesBorins
test: fix flaky http-writable-true-after-close
7998058 
PR-URL: #17952
Fixes: #16321
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
Merged
@TimothyGu TimothyGu removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 13, 2018
MylesBorins pushed a commit that referenced this pull request Jan 24, 2018
@apapirovski @MylesBorins
test: fix flaky http-writable-true-after-close
5eadfc8 
PR-URL: #17952
Fixes: #16321
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@lpinca lpinca lpinca approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OSX: sequential/test-http-writable-true-after-close
7 participants
@apapirovski @lpinca @mcollina @BridgeAR @MylesBorins @TimothyGu @nodejs-github-bot